Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

to 1.1.30: feat: add consistent logging at INFO and DEBUG levels #930

Open
wants to merge 4 commits into
base: v1.1.30
Choose a base branch
from

Conversation

kentbull
Copy link
Contributor

The goal is to make viewing the logs at the INFO level nice when manually reviewing log output and to make the DEBUG output comprehensive for everything except escrow logging.

The goal is to make viewing the logs at the INFO level nice when manually reviewing log output and to make the DEBUG output comprehensive for everything except escrow logging.
Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment and request as the other log PRs

Most log messages are unique enough that a simple search of the code base finds the one you are looking for. And if they aren't, I'd rather update the few that are to be more specific / descriptive and avoid this bespoke cataloging of log messages.

@kentbull kentbull changed the title feat: add consistent logging at INFO and DEBUG levels to 1.1.30: feat: add consistent logging at INFO and DEBUG levels Jan 29, 2025
@kentbull
Copy link
Contributor Author

Most log messages are unique enough that a simple search

Yes, while this is accurate, it was very useful to me while debugging multisig to be able to read the log output without having to go search. The reason I added the bespoke cataloging of log messages was because the bespoke labels added context that helped me think through the problem without having to constantly search the codebase.

IMO it's a set of tradeoffs. Add a bit more length to log messages, gain the ability to quickly understand what is going on.

As the person who wrote many, if not most of them, them I can see why you would lean in favor of avoiding the bespoke labels because you have memorized the context and can follow the logic often without having to refer back to the codebase.

As a newcomer to the codebase I found it tremendously helpful to have that specific tagging in the label. I imagine such labels will help other newcomers quickly understand the meaning of the log messages.

@kentbull kentbull force-pushed the further-enhanced-logging branch from d41d53a to 9844b9d Compare February 4, 2025 01:30
@kentbull
Copy link
Contributor Author

kentbull commented Feb 5, 2025

In the latest commit I addressed the class name prefix comment and metadata comments from this morning's dev meeting. The class names are removed and the metadata format string is removed as library users will customize that themselves.

@kentbull kentbull force-pushed the further-enhanced-logging branch 2 times, most recently from 1fa72be to 96b095b Compare February 7, 2025 16:27
@kentbull kentbull force-pushed the further-enhanced-logging branch from 96b095b to 47f53de Compare February 7, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants